Javascript javascript1 week4/ruslana #121
Conversation
There was a problem hiding this comment.
Overall, looks good. I would suggest breaking your getReply() function up into multiple - it's common to have a "main" function that delegates different tasks to helper functions.
So for example, you could have functions like addTodo or setUserName etc. and call them from the getReply() function. This could help keep the length of the getReply() function down and help keep it readable at-a-glance.
| let todoList = []; | ||
|
|
||
| function getReply(command) { | ||
| const lowerCaseCommand = command.toLowerCase(); |
There was a problem hiding this comment.
Good thinking! It's always nice to make an effort to handle slightly incorrect user input, such as casing.
| lowerCaseCommand.startsWith('add ') && | ||
| lowerCaseCommand.includes(' to my todo') | ||
| ) { | ||
| const todo = command.slice(4, command.indexOf(' to my todo')); |
There was a problem hiding this comment.
It's not something super important to handle, but it's always good to think about edge cases and how our code might behave.
For example, what if someone wanted to add think of some things to add to my todo to their todo list? Their command would be:
add think of some things to add to my todo to my todo
Which is a little silly for sure! But it would be nice if our code handled it.
What the code will currently do is see the first instance of to my todo when we call command.indexOf("to my todo"). That means it will assign think of some things to add to the todo variable, whereas it would ideally assign think of some things to add to my todo instead.
We can fix this by using a slightly different method from indexOf(). Fortunately Javascript has a lastIndexOf() method for exactly this reason. Instead of returning the index of the first matching string, it will return the index of the last one!
Again - not a super important fix for now. The lesson is mostly just to keep thinking about our code and how it might break, so that we can make deliberate choices about whether we fix it!
| ) { | ||
| const todo = command.slice(7, command.indexOf(' from my todo')); | ||
| const index = todoList.indexOf(todo); | ||
| if (index > -1) { |
There was a problem hiding this comment.
Great catch. Calling .splice() with -1 as an argument would have very unexpected results!
| if (lowerCaseCommand === 'what day is it today?') { | ||
| const today = new Date(); | ||
| const day = today.getDate(); | ||
| const month = today.toLocaleString('default', { month: 'long' }); |
There was a problem hiding this comment.
Great use of some newer Javascript functionality - toLocaleString(). Consider using it for the whole date format though, not just the month!
| if (lowerCaseCommand.startsWith('what is ')) { | ||
| const mathExpression = command.slice(8); | ||
| try { | ||
| const result = eval(mathExpression); |
There was a problem hiding this comment.
While eval() is very useful for cases like this where we want to evaluate an expression, we need to be very careful about using it as developers.
Many sites will disable using it entirely these days, but consider what happens if the user types
what is alert(`hello ${userName}`)
That's right - this user input will cause the browser (if running in the browser) to pop up an alert dialog, and it's even got access to the value of any variables! Consider that this could be used to send information about the user to a malicious site, and you'll start to realise why eval() is generally considered best left alone!
That said, for this assignment, it's a great way to get started.
An alternative could be to parse some numbers and an operator (+, -, *, /) form the command string, and then perform a different operation based on the operator.
| lowerCaseCommand.includes(' minutes') | ||
| ) { | ||
| const minutes = parseInt(command.slice(16, command.indexOf(' minutes'))); | ||
| if (isNaN(minutes)) { |
Did all tasks from the class (01.12) as I was absent and did the main voice assistant task